Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support loadBalancerClass in service reconciliation #7706

Merged
merged 4 commits into from
Apr 11, 2024

Conversation

pebrc
Copy link
Collaborator

@pebrc pebrc commented Apr 10, 2024

Fixes #7691

From the k8s docs:

// loadBalancerClass is the class of the load balancer implementation this Service belongs to.
// If specified, the value of this field must be a label-style identifier, with an optional prefix,
// e.g. "internal-vip" or "example.com/internal-vip". Unprefixed names are reserved for end-users.
// This field can only be set when the Service type is 'LoadBalancer'. If not set, the default load
// balancer implementation is used, today this is typically done through the cloud provider integration,
// but should apply for any default implementation. If set, it is assumed that a load balancer
// implementation is watching for Services with a matching class. Any default load balancer
// implementation (e.g. cloud providers) should ignore Services that set this field.
// This field can only be set when creating or updating a Service to type 'LoadBalancer'.
// Once set, it can not be changed. This field will be wiped when a service is updated to a non 'LoadBalancer' type.

AWS EKS uses this field on EKS clusters > 1.24 that have upgraded to the new AWS LoadBalancer Controller

Behaviour this PR should implement:

  • default the loadBalancerClass from the server side values if no value is set in the ECK service spec
  • recreate the service if the value set in the ECK service spec differes from the server side value
  • allow resetting the value only if the type of the service is no longer LoadBalancer

@pebrc pebrc added the v2.13.0 label Apr 10, 2024
@botelastic botelastic bot added the triage label Apr 10, 2024
@pebrc pebrc added >bug Something isn't working and removed triage labels Apr 10, 2024
Copy link
Contributor

@barkbay barkbay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also wondering if we should not also update applyServerSideValues for ExternalTrafficPolicy and AllocateLoadBalancerNodePorts, I think needsUpdate(...) always returns true on my test cluster with the following diff:

  v1.ServiceSpec{
        ... // 8 identical fields
        LoadBalancerSourceRanges:      nil,
        ExternalName:                  "",
-       ExternalTrafficPolicy:         "",
+       ExternalTrafficPolicy:         "Cluster",
        HealthCheckNodePort:           0,
        PublishNotReadyAddresses:      false,
        SessionAffinityConfig:         nil,
        IPFamilies:                    {"IPv4"},
        IPFamilyPolicy:                &"SingleStack",
-       AllocateLoadBalancerNodePorts: nil,
+       AllocateLoadBalancerNodePorts: &true,
        LoadBalancerClass:             &"foo",
        InternalTrafficPolicy:         &"Cluster",
  }

@pebrc
Copy link
Collaborator Author

pebrc commented Apr 11, 2024

I'm also wondering if we should not also update applyServerSideValues for ExternalTrafficPolicy and AllocateLoadBalancerNodePorts

I think that's a good idea. I will add it to this PR

Copy link
Contributor

@barkbay barkbay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@pebrc pebrc merged commit a0ab4c5 into elastic:main Apr 11, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug Something isn't working v2.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Service xxx is invalid: spec.loadBalancerClass: Invalid value: "null"
2 participants